🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19073

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 3 Cancelled Jobs, 5 Unrelated Failures

As of commit 6a25889 with merge base e4ab34d ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@claude review

Claude finished @psiddh's task in 3m 20s —— View job

Review: Arm backend cortex-m55+int8 e2e wiring

• Gather context and read changed files
• Review diff against origin/main
• Provide review feedback

Overall this is a well-structured PR. Consolidating the e2e flow into run.sh so CI and user-facing docs exercise the same code path is a clear improvement. The cortex-m branch in run_fvp.sh is clean and the early-exit with its own PASS/FAIL checking makes sense since etdump/generic-error scraping doesn't apply to the semihosting flow.

A few items worth addressing:

1. --quantize is silently ignored by run.sh (medium)

test_cortex_m_e2e.sh:23 passes --quantize to run.sh, and the PR's test plan also uses it. However, run.sh's arg parser (lines 89–122) has no --quantize case — only --no_quantize. The unrecognized flag falls through to *) ;; and is silently swallowed. This works because quantize is the default, but it's misleading: a reader would assume --quantize is a real flag. If someone later changes the default, this would silently break.

Suggestion: either drop --quantize from the CI wrapper (since it's the default), or add --quantize as a recognized no-op/explicit flag in run.sh's parser.

Fix this →

2. Dummy-input file coupling between run.sh and run_fvp.sh (minor)

run.sh:370 creates fvp_dummy_input.bin next to the .bpte, and run_fvp.sh:117–118 asserts its existence. This is a contract across two scripts that isn't enforced by anything other than comments and ordering. If a future caller invokes run_fvp.sh --bundle=... without knowing about the dummy file requirement, they get a somewhat cryptic error.

Consider having run_fvp.sh create the dummy file itself when --bundle is specified — it has all the information needed (bundle_dir). This removes the cross-script coupling and makes run_fvp.sh self-contained for the cortex-m path.

3. $? in "Simulation complete" message is always 0 (nit, pre-existing)

At run_fvp.sh:133:

... | sed 's/\r$//' | tee ${log_file} || true
echo "[${BASH_SOURCE[0]}] Simulation complete, $?"

The || true ensures the pipeline always exits 0, so $? in the echo is always 0 — the FVP's actual exit code is lost. This is the same pattern used in the existing ethos-u55/u85 branches (lines 158, 171), so it's pre-existing, but it's worth noting since this branch does its own PASS/FAIL checking anyway and the message could be confusing during debugging.

4. check_setup() has no branch for cortex-m targets (minor)

run.sh:194–234: check_setup() handles ethos-u and vgf targets but has no explicit branch for cortex-m*. For cortex-m, it sources setup_path_script and returns 0 without toolchain checks or calling build_executorch.sh. This works because build_test_runner.sh (called at run.sh:365) internally invokes build_executorch.sh. But toolchain validation is skipped, so setup errors won't surface until deeper in the build. A cortex-m branch in check_setup (even just a toolchain hash check) would catch misconfigurations earlier.

5. FVP model selection uses cortex-m55* specifically (nit)

run_fvp.sh:58 matches cortex-m55* for FVP selection (Corstone-300), while line 108 uses the broader cortex-m* for the semihosting launch path. This is correct for now since only cortex-m55 is supported, but if cortex-m33 or other variants are added, line 58 would need updating while line 108 would already handle them. Worth a brief comment noting the intentional specificity, or matching on cortex-m* and selecting the FVP based on a more explicit mapping.

Summary: The PR cleanly consolidates the cortex-m e2e flow. Items 1 and 2 are the most actionable — the rest are nits or pre-existing patterns. Nice work reducing test_cortex_m_e2e.sh to a thin wrapper.

Accepting it to unblock

Looks like there is a merge conflict (--quantize is silently ignored by run.sh seems reasonable feedback from Claude)

Failures unrelated.